Reviewing files that changed from the base of the PR and between f4ed9a0 and 00a4cc5.

• contracts/deploy/00-chainlink-rng.ts (1 hunks)
• contracts/deploy/00-home-chain-arbitration-neo.ts (3 hunks)
• contracts/deploy/00-home-chain-arbitration.ts (2 hunks)
• contracts/deploy/00-home-chain-resolver.ts (0 hunks)
• contracts/deploy/00-rng.ts (2 hunks)
• contracts/deploy/change-sortition-module-rng.ts (1 hunks)
• contracts/src/arbitration/SortitionModule.sol (2 hunks)
• contracts/src/arbitration/SortitionModuleBase.sol (5 hunks)
• contracts/src/arbitration/SortitionModuleNeo.sol (2 hunks)
• contracts/src/rng/BlockhashRNG.sol (1 hunks)
• contracts/src/rng/ChainlinkRNG.sol (8 hunks)
• contracts/src/rng/IRNG.sol (1 hunks)
• contracts/src/rng/IncrementalNG.sol (1 hunks)
• contracts/src/rng/RNG.sol (0 hunks)
• contracts/src/rng/RNGWithFallback.sol (1 hunks)
• contracts/src/rng/RandomizerRNG.sol (6 hunks)
• contracts/test/arbitration/dispute-kit-gated.ts (1 hunks)
• contracts/test/arbitration/draw.ts (1 hunks)
• contracts/test/arbitration/staking-neo.ts (2 hunks)
• contracts/test/arbitration/staking.ts (3 hunks)
• contracts/test/foundry/KlerosCore.t.sol (34 hunks)
• contracts/test/integration/index.ts (1 hunks)
• contracts/test/proxy/index.ts (1 hunks)
• contracts/test/rng/index.ts (5 hunks)
• contracts/test/utils/getActualAddress.ts (1 hunks)
• cspell.json (1 hunks)

• contracts/deploy/00-home-chain-resolver.ts
• contracts/src/rng/RNG.sol

• GitHub Check: Redirect rules - kleros-v2-testnet-devtools
• GitHub Check: Redirect rules - kleros-v2-neo
• GitHub Check: Header rules - kleros-v2-testnet-devtools
• GitHub Check: Redirect rules - kleros-v2-testnet
• GitHub Check: Redirect rules - kleros-v2-testnet
• GitHub Check: Header rules - kleros-v2-neo
• GitHub Check: Pages changed - kleros-v2-testnet-devtools
• GitHub Check: Header rules - kleros-v2-testnet
• GitHub Check: Header rules - kleros-v2-testnet
• GitHub Check: Pages changed - kleros-v2-neo
• GitHub Check: Pages changed - kleros-v2-testnet
• GitHub Check: Pages changed - kleros-v2-testnet
• GitHub Check: SonarCloud
• GitHub Check: Analyze (javascript)
• GitHub Check: contracts-testing

contracts/test/proxy/index.ts (1)

7-8: Avoid deep-linking into typechain-types – prefer barrel exports for stability

Pointing directly to ../../typechain-types/src/proxy/mock/by-rewrite/UpgradedByRewrite.sol makes the test depend on the internal folder structure generated by TypeChain.
A future refactor (e.g. changing outDir, renaming the solidity file, or upgrading TypeChain) will break this path silently while the re-exported barrel ../../typechain-types would stay intact.

Two low-friction options:

-import { UpgradedByRewrite as UpgradedByRewriteV1 } from "../../typechain-types/src/proxy/mock/by-rewrite/UpgradedByRewrite.sol";
+import type { UpgradedByRewrite as UpgradedByRewriteV1 } from "../../typechain-types";

or add a manual re-export in typechain-types/index.ts during code-gen to expose the duplicate names cleanly (e.g. UpgradedByRewriteV1, UpgradedByRewriteV2).

Either keeps the import resilient and removes the brittle deep path.
[ suggest_optional_refactor ]

cspell.json (1)

38-38: LGTM! Necessary addition for the RNG interface refactoring.

Adding "IRNG" to the spell checker dictionary is correct and necessary to support the interface rename from RNG to IRNG throughout the codebase.

contracts/deploy/00-chainlink-rng.ts (1)

74-74: Good addition! Helpful deployment guidance.

Adding the console message with the Chainlink VRF registration link provides useful post-deployment guidance for users.

contracts/test/utils/getActualAddress.ts (1)

11-11: LGTM! Updated to modern import assertion syntax.

The change from assert: { type: "json" } to with: { type: "json" } aligns with the current JavaScript/TypeScript standard for import assertions.

contracts/test/arbitration/dispute-kit-gated.ts (2)

64-64: LGTM! Correctly updated for simplified RNG interface.

The method call now correctly passes only the RNG address, reflecting the removal of the RNG lookahead parameter from the interface.

---

141-144: LGTM! Simplified block mining logic.

The removal of the RNG lookahead-based block mining loop and replacement with single block mining correctly reflects the interface changes where timing is now based on timestamps rather than block lookahead.

contracts/test/arbitration/staking-neo.ts (2)

59-65: LGTM! Improved TypeScript type safety.

Using generic type parameters with ethers.getContract<T>() is cleaner and provides better type safety than explicit type assertions.

---

108-108: LGTM! Correctly simplified block mining logic.

The change from mining multiple blocks based on RNG lookahead to mining a single block correctly reflects the removal of the lookahead parameter from the RNG interface.

contracts/test/arbitration/draw.ts (2)

84-84: LGTM: Simplified RNG configuration call

The removal of the second argument from changeRandomNumberGenerator correctly aligns with the new IRNG interface that eliminates the lookahead parameter.

---

177-177: LGTM: Simplified phase transition logic

The removal of the block mining loop that previously handled RNG lookahead is correct. The new IRNG interface no longer requires block-based timing, making this simplification appropriate.

contracts/src/rng/IRNG.sol (1)

1-13: LGTM: Well-designed RNG interface

The new IRNG interface is clean and simplified compared to the previous RNG interface. The removal of block-based parameters and the clear function signatures align well with the PR objectives. The documentation is appropriate and the interface follows Solidity best practices.

contracts/deploy/change-sortition-module-rng.ts (2)

26-27: LGTM: Updated to use new consumer role method

The change from changeSortitionModule to changeConsumer correctly reflects the refactoring of RNG contracts where the authorized role was renamed from sortitionModule to consumer.

---

29-30: LGTM: Simplified RNG configuration

The removal of the second argument from changeRandomNumberGenerator is consistent with the elimination of the lookahead parameter in the new IRNG interface.

contracts/deploy/00-rng.ts (3)

10-10: LGTM: Transition to time-based lookahead

The change from RNG_LOOKAHEAD (block-based) to RNG_LOOKAHEAD_TIME (time-based) with a 30-minute value aligns with the RNG refactoring that moves away from block-based timing mechanisms.

---

34-38: LGTM: Updated BlockHashRNG constructor arguments

The constructor arguments correctly reflect the new BlockHashRNG interface that takes a governor, consumer, and time-based lookahead parameter instead of the previous block-based approach.

---

42-42: LGTM: Simplified sortition module configuration

The single-argument call to changeRandomNumberGenerator is consistent with the removal of the lookahead parameter from the sortition module interface.

contracts/deploy/00-home-chain-arbitration.ts (2)

55-55: LGTM: Updated SortitionModule constructor

The removal of the RNG lookahead parameter from the SortitionModule constructor correctly aligns with the interface changes that eliminate block-based timing dependencies.

---

88-93: LGTM: Improved RNG consumer management

The change to use changeConsumer instead of changeSortitionModule is correct, and the conditional check before calling the method is a good optimization that avoids unnecessary transactions.

contracts/test/integration/index.ts (1)

206-208: LGTM! Improved type safety with explicit parameter typing.

The explicit typing of the function parameter enhances type safety and makes the expected structure clear, which is a good practice in TypeScript.

contracts/src/arbitration/SortitionModule.sol (3)

5-5: LGTM! Interface abstraction improvement.

The change from concrete RNG type to IRNG interface provides better abstraction and aligns with the PR's objective to introduce a composite RNG with fallback logic.

---

32-32: LGTM! Parameter updated to match new interface.

The change from RNG to IRNG parameter and removal of _rngLookahead aligns with the PR objectives to use the new IRNG interface and remove block-based lookahead logic.

---

34-34: LGTM! Base initializer call updated correctly.

The removal of _rngLookahead parameter from the base initializer call is consistent with the function signature changes and the broader refactor to remove block-based RNG timing.

contracts/deploy/00-home-chain-arbitration-neo.ts (3)

13-13: LGTM! Cleanup of unused deployment method.

Removing the unused execute method from the destructured deployments object improves code clarity by only importing what's needed.

---

52-52: LGTM! Deployment arguments updated to match contract changes.

The removal of RNG_LOOKAHEAD parameter from the SortitionModuleNeo deployment arguments correctly aligns with the contract's updated initialize() function signature.

---

86-91: LGTM! RNG linkage updated to use consumer pattern.

The changes from changeSortitionModule/sortitionModule() to changeConsumer/consumer() correctly implement the new RNG consumer role pattern mentioned in the PR objectives.

contracts/test/arbitration/staking.ts (3)

30-34: LGTM! Improved TypeScript contract retrieval with generics.

Using TypeScript generics instead of type casting provides better type safety and is more idiomatic. This is a good improvement over the previous as ContractType approach.

---

57-57: LGTM! Block mining simplified after RNG lookahead removal.

The single evm_mine call replaces the previous lookahead-based block mining logic, correctly reflecting the shift from block-based to time-based randomness readiness.

---

394-395: LGTM! Consistent simplification of block mining logic.

The single block mine after phase transition maintains consistency with the updated RNG timing model throughout the test suite.

contracts/src/arbitration/SortitionModuleNeo.sol (3)

5-5: LGTM! Import statement updated consistently with interface refactor.

The change to import IRNG interface instead of concrete RNG type follows the same pattern as SortitionModule.sol and supports the overall interface abstraction effort.

---

42-42: LGTM! Parameter signature updated consistently with base module.

The change from RNG to IRNG parameter and removal of _rngLookahead maintains consistency with SortitionModule.sol while preserving the Neo-specific parameters.

---

46-46: LGTM! Base initializer call updated to match interface changes.

The removal of _rngLookahead parameter from the base initializer call is consistent with the updated SortitionModuleBase interface and follows the same pattern as SortitionModule.sol.

contracts/src/rng/IncrementalNG.sol (4)

4-4: LGTM! Interface import updated correctly.

The import has been properly updated to use the new IRNG interface.

---

6-9: LGTM! Contract declaration and comments updated appropriately.

The contract now correctly implements the IRNG interface instead of inheriting from the deprecated RNG class. The comments have been updated to reflect the simplified interface.

---

17-19: LGTM! Function signature simplified correctly.

The requestRandomness() function signature has been properly simplified by removing the unused _block parameter, aligning with the new IRNG interface. The no-op implementation remains appropriate for this testing contract.

---

23-27: LGTM! Function signature simplified correctly.

The receiveRandomness() function signature has been properly simplified by removing the unused _block parameter. The incremental logic remains intact and appropriate for testing purposes.

contracts/src/arbitration/SortitionModuleBase.sol (5)

10-10: LGTM! Import updated to new IRNG interface.

The import has been correctly updated to use the new IRNG interface.

---

54-54: LGTM! Storage variable type updated correctly.

The rng storage variable has been properly updated to use the IRNG interface type.

---

105-105: LGTM! Constructor parameter updated correctly.

The constructor parameter has been properly updated to accept an IRNG instance instead of the deprecated RNG type.

---

152-159: LGTM! Function updated with simplified RNG interface.

The changeRandomNumberGenerator function has been correctly updated:

• Parameter type changed to IRNG
• Function name and comments updated appropriately
• Call to requestRandomness() simplified by removing the block parameter

The logic maintains the same behavior while using the simplified interface.

---

172-172: LGTM! RNG method calls simplified correctly.

The calls to requestRandomness() and receiveRandomness() have been properly simplified by removing the block parameters, aligning with the new IRNG interface. This removes the unnecessary block-based timing complexity while maintaining the same phase transition logic.

Also applies to: 175-175

contracts/test/foundry/KlerosCore.t.sol (6)

55-55: Comment accurately reflects the time-based approach.

The comment update from block-based to time-based properly documents the semantic change in the RNG lookahead mechanism.

---

95-96: Significant increase in RNG lookahead time.

The rngLookahead value has been increased from 20 to 600 seconds (10 minutes), which is a more realistic timing for RNG operations. This aligns with the shift to timestamp-based randomness and should provide adequate time for RNG fulfillment in production-like scenarios.

---

106-111: Simplified SortitionModule initialization.

The removal of rngLookahead from the SortitionModule initialization parameters correctly reflects the architectural change where RNG timing is now managed at the RNG contract level rather than the consumer level.

---

116-117: Proper RNG consumer linkage with governor privileges.

The explicit changeConsumer call with governor privileges correctly implements the new RNG permission model, ensuring only authorized consumers can request randomness.

---

287-298: Consistent RNG initialization pattern in event tests.

The initialization sequence correctly follows the new pattern: first initialize the SortitionModule, then link it as the RNG consumer with governor privileges.

---

1006-1006: Consistent implementation of timestamp-based RNG readiness.

All instances of vm.roll(block.number + rngLookahead + 1) have been correctly replaced with vm.warp(block.timestamp + rngLookahead), implementing the shift from block-based to timestamp-based RNG mechanics throughout the test suite.

Also applies to: 1053-1053, 1080-1080, 1136-1136, 1424-1424, 1483-1483, 1526-1526, 1636-1636, 1661-1661, 1772-1772, 1799-1799, 1840-1840, 1889-1889, 1979-1979, 2023-2023, 2119-2119, 2197-2197, 2231-2231, 2243-2243, 2349-2349, 2424-2424, 2470-2470, 2525-2525, 2615-2615, 2669-2669, 2708-2708, 2762-2762, 2805-2805, 2899-2899

contracts/test/rng/index.ts (3)

24-30: LGTM! Test correctly updated for new IRNG interface.

The removal of numeric arguments from receiveRandomness calls aligns with the simplified IRNG interface.

---

96-109: LGTM! ChainlinkRNG tests properly updated.

Good use of the shared abiCoder instance and correct removal of unused parameters.

---

155-202: LGTM! RandomizerRNG tests correctly updated.

Tests properly handle the simplified interface and maintain good coverage for both single and multiple request scenarios.

contracts/src/rng/RandomizerRNG.sol (3)

5-10: LGTM! Correct interface migration to IRNG.

The contract properly implements the new IRNG interface as part of the broader RNG refactoring.

---

16-16: LGTM! Consumer role properly implemented.

The migration from sortitionModule to consumer is complete and consistent throughout the contract, maintaining proper access control.

Also applies to: 44-47, 54-62, 74-78

---

102-107: LGTM! Randomness functions correctly simplified.

The removal of block parameters aligns with the IRNG interface. The lack of concurrent request handling is acceptable as confirmed by previous learnings that only the SortitionModule (now consumer) calls these functions without concurrent requests.

Also applies to: 124-126

contracts/src/rng/ChainlinkRNG.sol (3)

8-12: LGTM! Proper interface implementation and consistent event naming.

The contract correctly implements IRNG while maintaining Chainlink VRF integration through VRFConsumerBaseV2Plus inheritance. Event parameter naming is consistent.

Also applies to: 32-38

---

18-18: LGTM! Consumer role properly implemented with correct access control.

The migration from sortitionModule to consumer is complete and maintains proper authorization checks.

Also applies to: 49-52, 94-98

---

135-153: LGTM! VRF v2.5 integration with native ETH payment.

The implementation correctly uses Chainlink VRF v2.5 with native ETH payment enabled. The lack of concurrent request handling aligns with the design constraint that only one consumer makes requests.

contracts/src/rng/BlockhashRNG.sol (3)

13-50: LGTM! Well-structured contract with appropriate L2 considerations.

Good use of immutable for lookaheadTime and clear role separation. The timestamp-based approach is appropriate for L2 environments with sporadic block production.

---

109-119: LGTM! Useful view functions for checking randomness readiness.

The view functions properly handle the case where no request has been made and provide clear information about when randomness will be available.

---

79-101: Ignore blockhash and timestamp‐collision concerns

The two raised issues don’t apply here:

• Using blockhash(block.number - 1) always returns the hash of the immediately previous block, which is guaranteed to be within the last 256 blocks, so it will never be 0 due to the EVM’s 256-block limit.
• The contract only tracks a single outstanding request via requestTimestamp (overwritten on each call) and restricts calls to the designated consumer, so you cannot have concurrent requests mapping to the same key—no collision risk.

No changes needed.

Likely an incorrect or invalid review comment.

contracts/src/rng/RNGWithFallback.sol (5)

42-49: LGTM! Proper initialization with input validation.

Good validation of the default RNG address and proper initialization of the RNG array.

---

147-158: Missing validation in removeLastRngFallback.

The function doesn't check if the RNG being removed has any pending requests or if it's currently being used. This could lead to failed randomness retrieval.

Consider adding validation to ensure safe removal.

 function removeLastRngFallback() external onlyByGovernor {
     require(rngs.length > 1, "No fallback RNG");
+    require(currentRngIndex < rngs.length - 1, "Cannot remove active RNG");

⛔ Skipped due to learnings

Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.

Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.

---

129-136: Risk in changeDefaultRng when taking over pending requests.

Calling _requestRandomness(DEFAULT_RNG) during RNG change could fail if the new RNG is not ready or properly configured, potentially leaving the contract in an inconsistent state.

Consider adding try-catch or validation before attempting to take over requests.

⛔ Skipped due to learnings

Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.

Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.

---

84-109: Issues with fallback mechanism in receiveRandomness.

Several concerns:

1. Makes external calls without reentrancy protection
2. When falling back, immediately calls _requestRandomness which might fail if the next RNG requires specific conditions
3. No validation that the fallback RNG is operational before switching

Consider adding more robust error handling and validation of RNG availability.

⛔ Skipped due to learnings

Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.

Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.

---

70-80: Potential reentrancy vulnerability in requestRandomness.

The contract updates state variables before making an external call to rngs[_rngIndex].requestRandomness(). A malicious RNG could potentially call back into this contract.

Consider using reentrancy guards or following checks-effects-interactions pattern more strictly.

+import "@openzeppelin/contracts/utils/ReentrancyGuard.sol";

-contract RNGWithFallback is IRNG {
+contract RNGWithFallback is IRNG, ReentrancyGuard {

-    function requestRandomness() external override onlyByConsumer {
+    function requestRandomness() external override onlyByConsumer nonReentrant {

-    function receiveRandomness() external override onlyByConsumer returns (uint256 randomNumber) {
+    function receiveRandomness() external override onlyByConsumer nonReentrant returns (uint256 randomNumber) {

⛔ Skipped due to learnings

Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.

Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.

Reviewing files that changed from the base of the PR and between 5db2a51 and 8d9d2cb.

• contracts/deploy/00-chainlink-rng.ts (1 hunks)
• contracts/deploy/00-home-chain-arbitration-neo.ts (3 hunks)
• contracts/deploy/00-home-chain-arbitration.ts (2 hunks)
• contracts/deploy/00-home-chain-resolver.ts (0 hunks)
• contracts/deploy/00-rng.ts (2 hunks)
• contracts/deploy/change-sortition-module-rng.ts (1 hunks)
• contracts/src/arbitration/KlerosCoreNeo.sol (3 hunks)
• contracts/src/arbitration/SortitionModule.sol (2 hunks)
• contracts/src/arbitration/SortitionModuleBase.sol (5 hunks)
• contracts/src/arbitration/SortitionModuleNeo.sol (2 hunks)
• contracts/src/rng/BlockhashRNG.sol (1 hunks)
• contracts/src/rng/ChainlinkRNG.sol (8 hunks)
• contracts/src/rng/IRNG.sol (1 hunks)
• contracts/src/rng/IncrementalNG.sol (1 hunks)
• contracts/src/rng/RNG.sol (0 hunks)
• contracts/src/rng/RNGWithFallback.sol (1 hunks)
• contracts/src/rng/RandomizerRNG.sol (6 hunks)
• contracts/test/arbitration/dispute-kit-gated.ts (1 hunks)
• contracts/test/arbitration/draw.ts (1 hunks)
• contracts/test/arbitration/staking-neo.ts (2 hunks)
• contracts/test/arbitration/staking.ts (3 hunks)
• contracts/test/foundry/KlerosCore.t.sol (35 hunks)
• contracts/test/integration/index.ts (1 hunks)
• contracts/test/proxy/index.ts (1 hunks)
• contracts/test/rng/index.ts (5 hunks)
• cspell.json (1 hunks)

• contracts/deploy/00-home-chain-resolver.ts
• contracts/src/rng/RNG.sol

• contracts/deploy/00-chainlink-rng.ts
• contracts/src/rng/IRNG.sol
• contracts/test/proxy/index.ts
• contracts/test/rng/index.ts
• contracts/src/arbitration/KlerosCoreNeo.sol

• cspell.json
• contracts/test/arbitration/dispute-kit-gated.ts
• contracts/deploy/change-sortition-module-rng.ts
• contracts/deploy/00-rng.ts
• contracts/deploy/00-home-chain-arbitration.ts
• contracts/test/integration/index.ts
• contracts/test/arbitration/draw.ts
• contracts/test/arbitration/staking.ts
• contracts/test/arbitration/staking-neo.ts
• contracts/src/rng/IncrementalNG.sol
• contracts/deploy/00-home-chain-arbitration-neo.ts
• contracts/src/arbitration/SortitionModuleBase.sol
• contracts/test/foundry/KlerosCore.t.sol
• contracts/src/arbitration/SortitionModuleNeo.sol
• contracts/src/arbitration/SortitionModule.sol
• contracts/src/rng/RandomizerRNG.sol
• contracts/src/rng/ChainlinkRNG.sol

• GitHub Check: Redirect rules - kleros-v2-university
• GitHub Check: Header rules - kleros-v2-university
• GitHub Check: Pages changed - kleros-v2-university
• GitHub Check: Redirect rules - kleros-v2-testnet
• GitHub Check: Redirect rules - kleros-v2-neo
• GitHub Check: Redirect rules - kleros-v2-testnet
• GitHub Check: Header rules - kleros-v2-neo
• GitHub Check: Header rules - kleros-v2-testnet
• GitHub Check: Header rules - kleros-v2-testnet
• GitHub Check: Pages changed - kleros-v2-neo
• GitHub Check: Pages changed - kleros-v2-testnet
• GitHub Check: Analyze (javascript)
• GitHub Check: contracts-testing
• GitHub Check: Pages changed - kleros-v2-testnet

contracts/src/rng/BlockhashRNG.sol (7)

5-5: LGTM: Import aligns with new interface pattern.

The import of IRNG.sol correctly replaces the previous RNG interface inheritance.

---

10-22: Excellent storage design for L2 compatibility.

The transition from block-based to timestamp-based randomness is well-architected:

• Clear separation of governor/consumer roles
• lookaheadTime as immutable provides gas efficiency
• Random number caching via mapping enables future retrieval beyond blockhash window
• L2 compatibility rationale is well-documented

---

28-36: Proper access control implementation.

The modifiers correctly implement role-based access control with clear error messages and standard Solidity patterns.

---

72-75: LGTM: Simple and effective request implementation.

The function correctly implements the timestamp-based approach for L2 compatibility. Overwriting previous requests is acceptable given the single-consumer model.

---

77-101: Excellent implementation with comprehensive edge case handling.

The function properly handles all scenarios:

• Early validation for missing requests and timing constraints
• Efficient caching mechanism to preserve randomness beyond blockhash window
• Correct use of blockhash(block.number - 1) for randomness generation
• Graceful degradation returning 0 when randomness unavailable

The timestamp-based approach with caching elegantly solves L2 block production irregularities.

---

107-112: LGTM: Clear readiness check implementation.

The function correctly mirrors the timing logic from receiveRandomness and provides useful external visibility.

---

114-119: LGTM: Consistent timestamp calculation.

The function provides consistent timing information that aligns with the core randomness logic.

contracts/src/rng/RNGWithFallback.sol (5)

1-11: LGTM!

The contract structure, imports, and interface implementation are well-organized and follow Solidity best practices.

---

13-17: LGTM!

The state variables are well-designed with appropriate visibility and immutability where needed. Making rng immutable is a good security practice.

---

23-24: LGTM!

The events are well-designed for monitoring fallback occurrences and governance changes.

---

47-55: LGTM!

The access control modifiers are implemented correctly with clear error messages.

---

62-65: Add request state validation.

The function should validate that no request is already pending to prevent overwriting requestTimestamp and align with the PR objective of preventing repeated calls.

 function requestRandomness() external override onlyByConsumer {
+    require(requestTimestamp == 0, "Request already pending");
     requestTimestamp = block.timestamp;
     rng.requestRandomness();
 }

⛔ Skipped due to learnings

Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/ChainlinkRNG.sol:135-153
Timestamp: 2024-12-07T11:39:10.927Z
Learning: In the `contracts/src/rng/ChainlinkRNG.sol` contract, the `requestRandomness` function is restricted to being called only by the `SortitionModule`, which does not send concurrent requests. Therefore, it's acceptable not to handle multiple random number requests concurrently in this context.

Learnt from: jaybuidl
PR: kleros/kleros-v2#1778
File: contracts/src/rng/RandomizerRNG.sol:21-21
Timestamp: 2024-12-07T11:41:15.835Z
Learning: Only the `SortitionModule` can call `requestRandomness`, and it doesn't send concurrent requests, so handling multiple concurrent random number requests is unnecessary in this context.

Reviewing files that changed from the base of the PR and between 8d9d2cb and 937fb97.

• contracts/deploy/00-chainlink-rng.ts (1 hunks)
• contracts/deploy/00-home-chain-arbitration-neo.ts (2 hunks)
• contracts/deploy/00-home-chain-arbitration.ts (2 hunks)
• contracts/deploy/00-home-chain-resolver.ts (0 hunks)
• contracts/deploy/00-rng.ts (2 hunks)
• contracts/deploy/change-sortition-module-rng.ts (1 hunks)
• contracts/src/arbitration/SortitionModule.sol (2 hunks)
• contracts/src/arbitration/SortitionModuleBase.sol (5 hunks)
• contracts/src/arbitration/SortitionModuleNeo.sol (2 hunks)
• contracts/src/rng/BlockhashRNG.sol (1 hunks)
• contracts/src/rng/ChainlinkRNG.sol (8 hunks)
• contracts/src/rng/IRNG.sol (1 hunks)
• contracts/src/rng/IncrementalNG.sol (1 hunks)
• contracts/src/rng/RNG.sol (0 hunks)
• contracts/src/rng/RNGWithFallback.sol (1 hunks)
• contracts/src/rng/RandomizerRNG.sol (6 hunks)
• contracts/test/arbitration/dispute-kit-gated.ts (1 hunks)
• contracts/test/arbitration/draw.ts (1 hunks)
• contracts/test/arbitration/staking-neo.ts (2 hunks)
• contracts/test/arbitration/staking.ts (3 hunks)
• contracts/test/foundry/KlerosCore.t.sol (35 hunks)
• contracts/test/integration/index.ts (1 hunks)
• contracts/test/proxy/index.ts (1 hunks)
• contracts/test/rng/index.ts (5 hunks)
• cspell.json (1 hunks)

• contracts/deploy/00-home-chain-resolver.ts
• contracts/src/rng/RNG.sol

• contracts/test/proxy/index.ts
• contracts/deploy/00-chainlink-rng.ts

• cspell.json
• contracts/test/arbitration/staking-neo.ts
• contracts/src/arbitration/SortitionModuleNeo.sol
• contracts/src/rng/IRNG.sol
• contracts/test/arbitration/draw.ts
• contracts/src/rng/RandomizerRNG.sol
• contracts/deploy/00-home-chain-arbitration-neo.ts
• contracts/deploy/00-rng.ts
• contracts/src/rng/IncrementalNG.sol
• contracts/test/arbitration/dispute-kit-gated.ts
• contracts/src/arbitration/SortitionModule.sol
• contracts/test/foundry/KlerosCore.t.sol
• contracts/test/arbitration/staking.ts
• contracts/src/rng/ChainlinkRNG.sol
• contracts/test/integration/index.ts
• contracts/deploy/00-home-chain-arbitration.ts
• contracts/deploy/change-sortition-module-rng.ts
• contracts/src/arbitration/SortitionModuleBase.sol
• contracts/test/rng/index.ts

• GitHub Check: contracts-testing
• GitHub Check: SonarCloud
• GitHub Check: Analyze (javascript)

contracts/src/rng/BlockhashRNG.sol (6)

5-5: LGTM! Correct implementation of new IRNG interface.

The import of IRNG and contract declaration correctly implement the new standardized interface, aligning with the broader RNG system refactor described in the PR objectives.

Also applies to: 13-13

---

18-22: Well-designed storage structure for timestamp-based randomness.

The storage variables correctly implement the new timestamp-based approach with appropriate access controls and caching mechanism. Using immutable for lookaheadTime is a good gas optimization.

---

28-36: LGTM! Proper access control implementation.

The modifiers correctly implement role-based access control with clear error messages, following standard Solidity patterns.

---

73-75: LGTM! Correct implementation of timestamp-based randomness request.

The function correctly implements the new IRNG interface and timestamp-based approach by recording the current block timestamp to initiate the lookahead period.

---

79-101: LGTM! Well-implemented timestamp-based randomness retrieval with appropriate caching.

The function correctly implements the new timing mechanism with proper checks for request status and time passage. The use of blockhash(block.number - 1) is correct since blockhash(block.number) would always return 0. The caching strategy appropriately handles zero values by not caching them, allowing retry attempts if randomness is temporarily unavailable.

---

109-119: LGTM! Useful view functions with proper edge case handling.

Both functions correctly implement their intended logic and properly handle the edge case where no request has been made. These functions provide valuable information for external callers to determine randomness readiness.

Reviewing files that changed from the base of the PR and between 937fb97 and 34f6a65.

• contracts/src/rng/IRNG.sol (1 hunks)

• GitHub Check: Redirect rules - kleros-v2-neo
• GitHub Check: Header rules - kleros-v2-neo
• GitHub Check: Pages changed - kleros-v2-neo
• GitHub Check: Redirect rules - kleros-v2-testnet
• GitHub Check: Redirect rules - kleros-v2-testnet
• GitHub Check: Redirect rules - kleros-v2-testnet-devtools
• GitHub Check: Header rules - kleros-v2-testnet
• GitHub Check: Header rules - kleros-v2-testnet
• GitHub Check: Header rules - kleros-v2-testnet-devtools
• GitHub Check: Analyze (javascript)
• GitHub Check: contracts-testing
• GitHub Check: Pages changed - kleros-v2-testnet
• GitHub Check: Pages changed - kleros-v2-testnet
• GitHub Check: Pages changed - kleros-v2-testnet-devtools

contracts/src/rng/IRNG.sol (1)

6-9: LGTM: Minimal, clear interface aligned with PR goals.

The two-method interface matches the new no-block-parameter design and suits the single-consumer assumption from previous decisions. Good abstraction boundary for implementers.

Reviewing files that changed from the base of the PR and between da7eea1 and a8f3415.

• contracts/deploy/00-ethereum-pnk.ts (0 hunks)
• contracts/deploy/00-home-chain-arbitration-neo.ts (4 hunks)
• contracts/deploy/00-home-chain-arbitration.ts (3 hunks)
• contracts/deploy/00-home-chain-pnk-faucet.ts (0 hunks)
• contracts/deploy/00-home-chain-resolver.ts (2 hunks)
• contracts/deploy/00-rng-chainlink.ts (3 hunks)
• contracts/deploy/00-rng-randomizer.ts (2 hunks)
• contracts/deploy/00-rng.ts (0 hunks)
• contracts/test/rng/index.ts (7 hunks)

• contracts/deploy/00-home-chain-pnk-faucet.ts
• contracts/deploy/00-ethereum-pnk.ts
• contracts/deploy/00-rng.ts

• GitHub Check: Redirect rules - kleros-v2-neo
• GitHub Check: Redirect rules - kleros-v2-university
• GitHub Check: Redirect rules - kleros-v2-testnet
• GitHub Check: Redirect rules - kleros-v2-testnet
• GitHub Check: Header rules - kleros-v2-university
• GitHub Check: Header rules - kleros-v2-testnet
• GitHub Check: Header rules - kleros-v2-neo
• GitHub Check: Header rules - kleros-v2-testnet
• GitHub Check: Pages changed - kleros-v2-university
• GitHub Check: Pages changed - kleros-v2-testnet
• GitHub Check: Pages changed - kleros-v2-testnet
• GitHub Check: contracts-testing
• GitHub Check: Pages changed - kleros-v2-neo
• GitHub Check: Analyze (javascript)

contracts/deploy/00-home-chain-resolver.ts (1)

18-22: Idempotent deploy of DisputeResolver looks correct

Using getContractOrDeploy with from/args/log preserves behavior and prevents accidental redeployments. No functional issues spotted.

contracts/deploy/00-home-chain-arbitration.ts (4)

9-9: Type import update aligns with RNGWithFallback integration

Importing RNGWithFallback here matches the new wiring in this script and across the PR. Looks good.

---

52-56: Correct wiring: SortitionModule now consumes RNGWithFallback

Passing rngWithFallback.target into SortitionModule aligns with the new IRNG consumer model and centralizes RNG logic via the fallback wrapper. Good move.

---

82-87: Typed DisputeKit retrieval + conditional changeCore is sound

Fetching the typed contract and only calling changeCore when needed avoids redundant txs. This is clean.

---

89-94: Conditional RNG consumer update is correct

Ensures RNGWithFallback’s consumer is the newly deployed SortitionModule without unnecessary transactions. This matches the single-consumer assumption from prior learnings.

contracts/deploy/00-home-chain-arbitration-neo.ts (3)

9-9: Type import update aligns with RNGWithFallback integration (Neo)

Importing RNGWithFallback is consistent with the new RNG consumer model.

---

47-58: Correct wiring: SortitionModuleNeo now consumes RNGWithFallback

Replacing rng.target with rngWithFallback.target and removing lookahead is consistent with the new IRNG interface and fallback design. Looks good.

---

95-100: Conditional RNG consumer update is correct (Neo)

Ensures RNGWithFallback’s consumer is set to SortitionModuleNeo only when needed. Matches the one-consumer constraint.

contracts/deploy/00-rng-chainlink.ts (1)

91-98: Consumer wiring is correct and idempotent

Setting the ChainlinkRNG consumer to RNGWithFallback only if needed avoids redundant transactions. This aligns with the single-consumer model.

contracts/deploy/00-rng-randomizer.ts (1)

45-51: Consumer wiring is correct and idempotent

The consumer change is only applied when necessary, which avoids redundant txs. Good.

contracts/test/rng/index.ts (12)

2-2: LGTM: Import additions for deployment functionality

The addition of deployments and getNamedAccounts imports aligns with the new deployment-based test setup approach used throughout the file.

---

14-14: LGTM: Global deployer variable for test consistency

Adding the global deployer variable enables consistent access to the deployer address across test suites.

---

25-31: LGTM: Transition to parameterless receiveRandomness API

The test correctly adapts to the new IRNG interface by using receiveRandomness() without parameters and staticCall to read the current value without state changes. The incremental behavior is properly validated.

---

39-46: LGTM: Deployment-based test setup with proper configuration

The switch to using Hardhat deployments with explicit constructor arguments (governor, consumer, lookaheadTime) provides better test isolation and aligns with the new deployment patterns. The 10-second lookahead time is appropriate for testing.

---

48-65: LGTM: Comprehensive test for time-based randomness readiness

This test properly validates the new request-driven flow:

• Requests randomness first
• Checks readiness state before and after time advancement
• Uses isRandomnessReady() to poll readiness status
• Validates non-zero random number generation

The test flow aligns well with the BlockHashRNG's time-based readiness mechanism.

---

67-82: LGTM: Edge case validation for BlockHashRNG

Both new test cases properly validate the expected behavior:

1. Returns 0 when no randomness has been requested
2. Returns 0 when insufficient time has elapsed

These tests ensure robustness and prevent incorrect usage patterns.

---

89-99: LGTM: Proper test setup with consumer configuration

The setup correctly:

• Uses getNamedAccounts() to obtain the deployer address
• Configures the RNG contract with changeConsumer(deployer) after deployment
• Follows the new consumer-based authorization pattern

---

103-114: LGTM: Streamlined ChainlinkRNG test with parameterless API

The test correctly adapts to the new interface:

• Uses parameterless requestRandomness()
• Uses parameterless receiveRandomness() to get the result
• Maintains proper event validation and expected value calculation using the shared abiCoder instance

---

118-144: LGTM: Multiple request handling validation

This test properly validates that only the last fulfilled randomness is returned when multiple requests are made, which aligns with the centralized fallback logic mentioned in the PR objectives. The test correctly uses the shared abiCoder instance for consistent expected value calculations.

---

152-162: LGTM: Consistent RandomizerRNG setup

The setup mirrors the ChainlinkRNG pattern with proper deployment fixture usage and consumer configuration, maintaining consistency across RNG implementations.

---

169-178: LGTM: RandomizerRNG test adaptation

The test correctly implements the parameterless API pattern while maintaining proper validation of the randomizer relay mechanism and expected random number generation.

---

182-211: LGTM: Consistent multiple request handling

This test maintains the same validation pattern as ChainlinkRNG for multiple requests, ensuring that only the last fulfilled randomness is returned. This consistency is important for the unified IRNG interface behavior.

Reviewing files that changed from the base of the PR and between a8f3415 and 831b062.

• contracts/src/arbitration/SortitionModuleBase.sol (5 hunks)
• contracts/test/arbitration/staking-neo.ts (2 hunks)
• contracts/test/foundry/KlerosCore.t.sol (35 hunks)

• contracts/test/arbitration/staking-neo.ts
• contracts/src/arbitration/SortitionModuleBase.sol

• GitHub Check: contracts-testing
• GitHub Check: Analyze (javascript)

contracts/test/foundry/KlerosCore.t.sol (4)

1020-1021: Consistent timestamp-based RNG readiness pattern

The pattern of using vm.warp(block.timestamp + rngLookahead) after calling passPhase() to reach the Generating phase is correctly applied throughout the test file. This aligns with the timestamp-based RNG readiness implementation.

Also applies to: 1067-1068, 1094-1095, 1150-1151, 1438-1439, 1474-1475, 1517-1518, 1560-1561, 1670-1671, 1695-1696, 1806-1807, 1833-1834, 1874-1875, 1923-1924, 2013-2014, 2057-2058, 2158-2159, 2236-2237, 2270-2271, 2282-2283, 2388-2389, 2463-2464, 2509-2510, 2564-2565, 2654-2655, 2708-2709, 2747-2748, 2801-2802, 2843-2844, 2943-2944

---

112-119: Simplified SortitionModule initialization

The SortitionModule initialization correctly uses the shortened signature without the rngLookahead parameter, passing only the rng address. This aligns with the PR's changes to remove lookahead from interfaces.

---

123-124: Consumer linkage after proxy creation

Good practice to set the RNG consumer after the sortition module proxy is created. This ensures the correct address is registered.

---

299-306: Consistent initialization pattern in events test

The initialization pattern in the events test correctly mirrors the main setup, maintaining consistency.

Reviewing files that changed from the base of the PR and between 831b062 and 277457c.

• contracts/src/test/RNGMock.sol (1 hunks)
• contracts/test/foundry/KlerosCore.t.sol (37 hunks)

• GitHub Check: Redirect rules - kleros-v2-testnet
• GitHub Check: Redirect rules - kleros-v2-neo
• GitHub Check: Redirect rules - kleros-v2-university
• GitHub Check: Redirect rules - kleros-v2-testnet
• GitHub Check: Header rules - kleros-v2-neo
• GitHub Check: Header rules - kleros-v2-testnet
• GitHub Check: contracts-testing
• GitHub Check: Analyze (javascript)
• GitHub Check: Header rules - kleros-v2-university
• GitHub Check: Header rules - kleros-v2-testnet
• GitHub Check: Pages changed - kleros-v2-neo
• GitHub Check: Pages changed - kleros-v2-testnet
• GitHub Check: Pages changed - kleros-v2-university
• GitHub Check: Pages changed - kleros-v2-testnet

contracts/src/test/RNGMock.sol (3)

1-5: LGTM on SPDX, pragma, and import path

License header, compiler version (^0.8.24), and the relative import path look correct for this repo layout.

---

6-19: Minimal, purpose-fit test double — looks good

Simple, deterministic mock implementing IRNG for fallback tests is appropriate. With optional events, this becomes even easier to assert against without adding state complexity.

---

7-18: IRNG interface coverage confirmed
The IRNG interface in contracts/src/rng/IRNG.sol declares only

• function requestRandomness() external;
• function receiveRandomness() external returns (uint256);

and RNGMock implements both with matching external visibility. The added view on receiveRandomness is permissible since it only reads state. No additional stubs are required for compilation.

contracts/test/foundry/KlerosCore.t.sol (4)

125-127: Good: consumer set after SortitionModule is deployed

Setting the consumer post-proxy initialization under governor privileges ensures the RNG is properly permissioned and avoids having to re-deploy RNG. This wiring is clean.

---

2998-3035: Verified RNG fallback event and blockhash source – test is correct

Both assumptions in the test are confirmed in RNGWithFallback.sol:

• The contract declares and emits event RNGFallback() when falling back.
• It falls back to uint256(blockhash(block.number - 1)) as the random source.

No changes needed.

---

3070-3105: Revert checks correctly match string-based requires

The RNGWithFallback contract uses
• require(msg.sender == governor, "Governor only") (line 48)
• require(msg.sender == consumer, "Consumer only") (line 53)

No custom errors are defined, so asserting exact string messages with

vm.expectRevert(bytes("Consumer only"));
vm.expectRevert(bytes("Governor only"));

is correct and need not be changed.

---

1022-1023: No off-by-one for RNG lookahead – ready check uses ≥

We verified in contracts/src/rng/BlockhashRNG.sol that readiness is checked with

return block.timestamp >= requestTimestamp + lookaheadTime;

so warping to exactly block.timestamp + rngLookahead satisfies the condition.
RNGWithFallback’s strict “>” only applies to its fallback timeout, which isn’t used by these lookahead-based tests. No changes required.